[WIP] Add experimental support for unified hosts #3817
Conversation
jh-db
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
jh-db
requested review from
mgyucht and
renaudhartert-db
and removed request for
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
|
cmd/auth/login.go
Outdated
| @@ -233,7 +241,7 @@ depends on the existing profiles you have set in your configuration file | |||
| // 1. --account-id flag. | |||
| // 2. account-id from the specified profile, if available. | |||
| // 3. Prompt the user for the account-id. | |||
| func setHostAndAccountId(ctx context.Context, existingProfile *profile.Profile, authArguments *auth.AuthArguments, args []string) error { | |||
| func setHostAndAccountId(ctx context.Context, cmd *cobra.Command, existingProfile *profile.Profile, authArguments *auth.AuthArguments, args []string) error { | |||
There was a problem hiding this comment.
No need to take cmd if you can stick this property in the auth arguments?
There was a problem hiding this comment.
Good call, I didn't notice because I added cmd first, and then changes authArguments later when I realized I needed it there.
| } | ||
|
|
||
| prompt := cmdio.Prompt(ctx) | ||
| prompt.Label = "Databricks workspace ID (optional - provide only if using this profile for workspace operations, leave empty for account operations)" |
There was a problem hiding this comment.
Is there not a way to detect this?
There was a problem hiding this comment.
Do you mean infer and decide whether this should be used or not based on the operation?
There was a problem hiding this comment.
Currently the cli api command and some code in TF relies on being able to tell whether a config is a workspace config or an account config before making a call. We originally wanted to make it possible to use the same profile/config for account and workspace calls, but since the code needs a way to distinguish them, we opted to make workspaceId required for unified workspace profiles, and forbid its presence in unified account profiles.
libs/auth/error.go
Outdated
| target := &u2m.InvalidRefreshTokenError{} | ||
| if errors.As(err, &target) { | ||
| oauthArgument, err := AuthArguments{host, accountId}.ToOAuthArgument() | ||
| oauthArgument, err := AuthArguments{host, accountId, false}.ToOAuthArgument() // TODO: pass the IsUnifiedHost flag |
There was a problem hiding this comment.
[nit] use field names, this makes the code more robust in case of refactoring.
| oauthArgument, err := AuthArguments{host, accountId, false}.ToOAuthArgument() // TODO: pass the IsUnifiedHost flag | |
| oauthArgument, err := AuthArguments{ | |
| Host: host, | |
| AccountID: accountId, | |
| Experimental_IsUnifiedHost: false, | |
| }.ToOAuthArgument() // TODO: pass the IsUnifiedHost flag |
libs/auth/arguments.go
Outdated
| } | ||
| host := cfg.CanonicalHostName() | ||
| if cfg.IsAccountClient() { | ||
| if cfg.GetHostType() == config.AccountHost { |
There was a problem hiding this comment.
Prefer exhaustive switch.
cmd/auth/login.go
Outdated
| @@ -233,7 +241,7 @@ depends on the existing profiles you have set in your configuration file | |||
| // 1. --account-id flag. | |||
| // 2. account-id from the specified profile, if available. | |||
| // 3. Prompt the user for the account-id. | |||
| func setHostAndAccountId(ctx context.Context, existingProfile *profile.Profile, authArguments *auth.AuthArguments, args []string) error { | |||
| func setHostAndAccountId(ctx context.Context, cmd *cobra.Command, existingProfile *profile.Profile, authArguments *auth.AuthArguments, args []string) error { | |||
jh-db
force-pushed
the
jh-db/unified-hosts
branch
from
8263dd2 to
9b5df04
Compare
jh-db
force-pushed
the
jh-db/unified-hosts
branch
from
9b5df04 to
8539eba
Compare
Changes
experimental_is_unified_hostflag to auth commands and persist it in profilesDepends on databricks/databricks-sdk-go#1307 and won't build without those SDK changes being released first
Why
Unified hosts support both workspace and accounts APIs. Support for this required making some changes to the Go SDK, which the cli relies on.
Tests
Not tested yet